Products may be created with pounds for their weight unit_converter #1
Conversation
|
Great start!
One of the questions I have is that the system is converting units on the database and storing a scale. I recommend you create a few variants with different units and look at how these database fields play together: spree_products.variant_unit, spree_products.variant_unit_scale and spree_variants.unit_value. Good luck! |
|
Hey @luisramos0 ! We were trying to make headway without bothering y'all since it didn't seem like a huge priority, but since your here; would you be willing to point at where we may want to add test-cases to validate that our change doesn't break things? My ideal test cases would cover:
(I'm not particularly fluent in open-food-network parlance so I'm most worried about creating opportunities for missing stairs in core workflows, so getting those paths figured out would make me feel a lot better about submitting this patch). |
|
Hello @zspencer great question! The first thing that came to my mind, although not exactly a primary workflow, was a fourth database field called final_weight_volume, this field is calculated most of the times based on the variant_unit_scale and unit value BUT there's this page under Orders called Bulk Order Management (admin/orders/bulk_management) where you have a column Weight/Volume that is very useful to adjust the final weight of a product (you order a large slice of cheese marked 500g and the hub will use this field to insert the exact final weight when packing). an update on this column should trigger the recalculation of the shipping cost and use the final_weight_volume if shipping cost is weight based. This should work correctly with the new units. The other case I can think of is changing the products unit type from items or volume to weight and back to items and verify nothing crazy happens to these fields. |
See: https://community.openfoodnetwork.org/t/hubs-managers-can-choose-the-adapted-weight-and-measure-units-for-their-shops-given-their-own-local-situation/1289/11 We're not entirely sure what needs to be changed in order for this to accurately work with shipping and other parts of the eCommerce platform. We are assuming that so long as we canonically store the weight scale in grams, that the shipping calculation will be able to do what it needs to. So if we put in values for "oz" as grams, we may not need to do much else in order to let product(s) be sold by the pound (or ounce). Next steps appear to be: - [ ] When looking at an order as a customer, do we want to show pounds instead of grams? (See: http://localhost:3000/orders/R125684626) - [ ] Compile a list of tests that are worth writing (because we have no confidence that we know what we are supposed to be doing in order for this feature to be "ready" to be used by people.) - [ ] Write a test that demonstrates when we create a product with a variant in pound that the product's shipping weight is correctly calculated? - [ ] Do we want to think about i18n?
We are pretty sure this is not the correct final implementation, but we wanted to get some tests failing so we can start to fix them.
This changes how we display the description of weight, but it doesn't change the `Spree::OptionValue`s that are being created when someone adds a product to their cart. This takes us closer by making the UI look more correct; but it feels odd compared to settiong the `Spree::OptionValue` to the correct unit on creation. But on the other hand, that could possibly make things worse for the shipping calculation bits.
zspencer
force-pushed
the
enhance/products-may-be-sold-in-imperial-units
branch
2 times, most recently
from
7bc1539
to
77ae4b4
Compare
|
@luisramos0 - OK, we're a bit further; but there are test failures still, so we aren't quite ready for more formal review. That said, we are curious if you think we're heading in the right direction here. For example, we think we may want to get rid of (or conditionalize) the change we made to the unit scaling since it appears to switch people to from g to oz to lb to kg to t; which... uhh... doesn't seem right. We're also wondering if there's a place where the logic we introduced to get the customer's order screen to show the correct value has a better place to live? For instance, if we're monkey patching |
There was a problem hiding this comment.
nice, I see you are making progress and you know what you are doing 💪
But I agree with you, this last commit is not good, you need to change the option_value when the unit is created/changed.
You need to understand how the option value is inserted when the variant is created. That will require reading some code in spree. Maybe here? https://github.com/openfoodfoundation/spree/blob/2-0-4-stable/core/app/models/spree/variant.rb
| # change the existing product(s), so if the scale value | ||
| # is changed, a data migration may be necessary to make sure | ||
| # the proper unit X to grams actually works. | ||
| # TODO: ^^^ Delete this |
There was a problem hiding this comment.
afaik this is only used in the UI when you change the unit of the product. All other cases are handled on the server side.
| # We have _no idea_ what this is doing. It has units? | ||
| # And it maybe is connected to something related to shipping? | ||
| # TODO: DELETE THIS ^^^ | ||
| 'lb' => { scale: 453.592, unit: 'weight' }, |
There was a problem hiding this comment.
this is unit conversion in the product import tool (menu Products > Import). It's just repeated logic that should be in a service 🙈 This refactoring is not your responsibility as you just landed on the codebase!
There was a problem hiding this comment.
Apologies, I've fallen behind on this. Thanks for this pointer!
| # shop or producer basis? | ||
|
|
||
| units = { 'weight' => { 1.0 => 'g', 1000.0 => 'kg', 1_000_000.0 => 'T', | ||
| 28.34952 => 'oz', 453.59237 => 'lb'}, |
There was a problem hiding this comment.
I think this is good, I think this is the most important bit 💪
There was a problem hiding this comment.
Glad we found it :D :D :D
There was a problem hiding this comment.
This does, indeed, cause the weird results where a 29g variant will try to use ounces, a 445g variant will use lbs, etc.
Would it be crazy to remove the autoscaling behavior instead of trying to write some conditional-heavy code here? That is, could we use the variant_unit_scale of the product as the scale for all variants, instead of adjusting the scale based on the variant?
I made a (wip) commit in order to test this on Semaphore; there are some test failures coming back related to it, but nothing we can't fix/rewrite. However, I want to see if such a change would be acceptable to the larger community first or if having the variants autoscale is important behavior.
From a code philosophy point of view, it seems to me to be much cleaner to rely on the product's scale as the single place where the scale information lives. And from the user perspective, it seems to me to be easy enough to denote most common cases in, for example, either g or kg, instead of assuming that the software will auto convert.
But @luisramos0 (or maybe you too Zee) will probably have a much better sense for whether such a change would be acceptable.
There was a problem hiding this comment.
This is a challenge I've ran into in other food-related systems (I built a nutrition-based recipe adjustment recommendation app for a startup a few years back). The ability to do implicit conversions is really slick from a user perspective; but it does add significant complexity to the underlying application logic.
The way we resolved that coplexity in the other project was by storing all the data in the database in milli(grams || liters), and performing the conversion at the user interface/ presentation layer.
I don't know enough about the OFN code-base to know how feasible or desirable such a solution is.
|
Hi @zspencer - have you made any progress since that last commit that's been pushed? I joined the OFN Slack channel and some people pointed me toward this issue since I'm also based in the US. I have this branch running locally and have my head wrapped around some of the relevant models, as well as some ideas for tests. But I don't want to step on any toes! If you're all right with me picking this up or contributing to it, would you like me to push any commits to the wecohere fork or cherry-pick and continue on my own fork? |
|
Hey @andrewpbrett, don't worry about stepping on my toes. I've got some pretty great steel toed boots I wear when working on projects :). That said, I have no made any additional progress. Life got away from me a bit these past few months! I'd be delighted if the progress we made was useful in your work; but it's definitely not required, so if you see a clearer trajectory to the solution that is along a different path then by all means take it! I'm also down to pair on it some weekend, send me an email zee@wecohere.com if you want to coordinate something like that? |
|
Thanks @zspencer - here's what I came up with. Should address the issue of 29g getting converted to ounces etc., and the shipping tests are passing, but I want to sanity check that area a bit more. |
|
YOU ROCK @andrewpbrett !!! <3 <3 <3 |
See: https://community.openfoodnetwork.org/t/hubs-managers-can-choose-the-adapted-weight-and-measure-units-for-their-shops-given-their-own-local-situation/1289/11
We're not entirely sure what needs to be changed in order for this to
accurately work with shipping and other parts of the eCommerce platform.
We are assuming that so long as we canonically store the weight scale
in grams, that the shipping calculation will be able to do what it needs
to. So if we put in values for "oz" as grams, we may not need to do
much else in order to let product(s) be sold by the pound (or ounce).
Next steps appear to be:
When looking at an order as a customer, do we want to show pounds
instead of grams? (See: http://localhost:3000/orders/R125684626)
Spree::OptionValueat the database level, or rely on presentation logic (which is what we're doing presently)Compile a list of tests that are worth writing (because we have
no confidence that we know what we are supposed to be doing in
order for this feature to be "ready" to be used by people.)
Write a test that demonstrates when we create a product with a
variant in pound that the product's shipping weight is correctly
calculated?
Do we want to think about i18n?
What? Why?
Closes #[the issue number this PR is related to]
What should we test?
Release notes
Changelog Category: Added | Changed | Deprecated | Removed | Fixed | Security
How is this related to the Spree upgrade?
Discourse thread
Dependencies
Documentation updates